feat(core): add masked_connector_response with per-connector key allowlist - #2050

there is a existing function for this functionality of removing prefixes i think we can use it

Agreed on the duplication, though strip_bom_and_convert_to_string (service.rs:1688) can't be called from here:

• it's private to external-services, and domain_types can't depend on that crate — the arrow runs the other way (external-services/Cargo.toml:46);
• signature mismatch: it's &[u8] -> Option<String>, but this point needs &[u8] -> &[u8]. detect() sniffs raw bytes and mask_xml/mask_form consume bytes, so routing through String would force UTF-8 validation plus a to_vec() copy before format detection, and drop non-UTF-8 bodies to the unparsable stub.

Extracted strip_utf8_bom(&[u8]) -> &[u8] into common_utils::bytes_utils instead — both crates already depend on common_utils. Both call sites now go through it, and it strips repeated BOMs to match the trim_start_matches behaviour it replaced, so service.rs is unchanged behaviourally.

I'm not sure we should own this hardcoded denylist — we can't realistically know every sensitive field name across all connectors. One connector calls it pan, another
primaryAccountNumber, another encryptedPaymentData. This list will always be playing catch-up.

Right now the allowlist is static config so it's manageable, but when this becomes dynamic (client-controlled), we should not be logging unmasked_connector_response on
our side at all — just return it in the gRPC response. That way if someone configures a bad allowlist, the unmasked value only shows up in their system, not ours.

Two parts here, answering them separately.

On the denylist — the premise is right, but it isn't the mechanism doing the work. The default is mask-everything, so an unknown sensitive field name is masked because it was never allowlisted, not because the denylist caught it. ALWAYS_MASKED_SUBSTRING/_EXACT only ever fire on keys an operator explicitly allowlisted, which makes them a backstop against a careless config entry rather than the primary defence. An incomplete list therefore costs nothing the default doesn't already cover, so I'd keep it as-is.

On the logging — taking this now rather than at the dynamic-config milestone, because the containment argument already holds under static config: a typo in the TOML writes to our logs today. Added log_to_span, gated separately from enabled:

[connector_response_masking]
enabled     = true   # populate the field on the gRPC response
log_to_span = false  # ALSO record response.unmasked_body on our span

true in development.toml, false in sandbox.toml and production.toml. The caller always gets the masked view back on the response; a copy only lands in our own logs where that's explicitly turned on, so a bad allowlist entry stays contained to whoever configured it.

add debug log here

Adding a log here would have papered over a live bug — the comment above this branch is wrong.

connector_name doesn't come from ConnectorEnum. It comes from ConnectorVariant::get_connector_name() (connector_types.rs:403), which stringifies five enums. Three names have no ConnectorEnum counterpart, so from_str fails and the function returns before populating anything:

The branch is reachable — set_unmasked_connector_response is implemented for surcharge_types.rs, payouts_types.rs and merchant_authentication_flow_data.rs. And the breakage was inconsistent rather than uniform: Loonio, Paypal, Itaubank, Worldpayxml, Cybersource (payout) and Kount (FRM) collide with ConnectorEnum names and worked fine, so behaviour differed per connector inside the same flow with no signal.

Same root cause hit config load: deserialize_connector_keys also parsed keys with ConnectorEnum::from_str, so deutschebank = "..." in a TOML aborted startup for a perfectly real connector.

Rather than logging the silent exit, I removed the re-parse. Ingress already validates the name per flow family (connector_variant_from_metadata, metadata.rs:113 — x-connector against ConnectorEnum, x-payout-connector against PayoutConnectorEnum, and so on), so the string arriving here is already canonical. It's now used directly as a lookup key, and config-load validation accepts a name that any of the five enums recognises. A lookup miss degrades to the empty allowlist — which is already the correct default for an unconfigured connector — so there's no failure branch left to log.

Verified: with interpayments/deutschebank/plaid as config keys the server boots; paysafee still aborts with unknown connector `paysafee` .

all these string in proto are masked_connector_response right?

Correct, and the name was backwards — renamed to masked_connector_response in 709b8509.

The field holds the connector reply with every value masked (***) except the ones an operator allowlisted for that connector, so unmasked_ promised the opposite of what it carries. Worse given the neighbour: these structs already have raw_connector_response, which is the verbatim unmasked body. Two fields, raw_ and unmasked_, and the safe one was the one whose name promised no masking.

Pure identifier rename, no behavioural change — 260 occurrences plus 5 of the response.unmasked_body span field, across 23 files:

unmasked_connector_response -> masked_connector_response
unmasked_body               -> masked_body

Proto tags are untouched, so the wire format is unchanged and only the generated accessor names move. Not a compatibility break either, since these fields don't exist on main — this PR introduces them.

Left alone deliberately: raw_connector_response (394 occurrences — the genuinely unmasked sibling), unmasked_headers (unrelated log config), and mask_connector_response / ConnectorResponseMaskingConfig / MASKED, which already describe masking correctly.

One side effect worth flagging: this also turns SDK Tests green, but by luck rather than by fixing anything. That job was failing because protoc emits every payment.proto message into a single outer class (java_multiple_files is unset) and the generated Payment.java hit 20,973,967 bytes against the Kotlin compiler's embedded IntelliJ cap of 20,971,520 — over by 2,447 bytes, surfacing as FileTooBigException → "Internal compiler error". The shorter identifier reclaims 3,006 bytes, landing ~559 bytes under. main alone sits ~1% below the same ceiling and arduino/setup-protoc@v3 is unpinned in ci.yml, so this will recur. Pinning protoc / raising kotlin.daemon.jvmargs is worth doing separately on main.

Anything that isn't JSON/XML/SOAP-shaped falls through to Format::Form, and mask_form round-trips arbitrary bytes as a single (whole_body, "") pair — so the body comes back out as a key, unmasked. A declared text/plain lands here too, since the Content-Type branch above doesn't recognise it.

"Transaction declined for card 4111111111111111"  ->  "Transaction+declined+for+card+4111111111111111=***"
"1,4111111111111111,SUPERSECRET"                  ->  "1%2C4111111111111111%2CSUPERSECRET=***"

The {"_format":"unparsable"} stub never fires because mask_form doesn't fail on arbitrary bytes. Can detect return None when neither the declared Content-Type nor the sniff matches a structured format?

Confirmed, and the stub was unreachable for the whole Form branch — serde_urlencoded never fails on arbitrary bytes, so mask_form always returned Some.

Both your examples reproduced exactly as written; they're now tests (a_plain_text_body_is_not_re_emitted_as_a_form_key, a_csv_shaped_body_is_not_re_emitted_as_form_keys, plus a binary body).

On where to put the check — I went one level lower than detect. A detect-only change still routes a declared application/x-www-form-urlencoded to the leaking path, and a connector declaring that header while returning something else is exactly the case worth catching. is_pair_shaped now gates mask_form, after newline normalisation so Fiuu's bodies are judged on their real pairs:

for segment in body.split(|byte| *byte == b'&') {
    if segment.is_empty() { continue; }          // trailing or doubled `&`
    match segment.iter().position(|byte| *byte == b'=') {
        None | Some(0) => return false,          // no `=`, or no key before it
        Some(_) => saw_pair = true,
    }
}

Non-form bodies now fall to {"_format":"unparsable","_bytes":N}. The sniff in detect stays, so a genuine form body with a missing Content-Type still works.

One trade-off worth naming: it's all-or-nothing, so a real form body with a valueless segment (a=1&flag&b=2) is stubbed whole rather than partly emitted. No gateway I know of sends that shape and losing a diagnostic is the cheaper failure, but say the word if you'd rather it degrade per-segment.

Fixed in d2eeb6a.

The root is seeded mask: false, so a body with no key to gate on goes out in the clear:

["4111111111111111","x"]  ->  ["4111111111111111","x"]
"4111111111111111"        ->  "4111111111111111"

Both reach here — [ sniffs as JSON, and a bare string with Content-Type: application/json routes here too. Seeding the root mask: true fixes it; objects re-decide per key one level down, so nothing correctly revealed today regresses.

Confirmed both, and the second one is worse than it looks: the seed inverts the fail-closed premise the whole design rests on, and it's invisible on the one shape every existing test used. An object re-decides per key immediately, so {...} bodies masked correctly no matter what the root was seeded with.

Seeded true in mask_json. Your two cases and the sniffed variant ([ with no Content-Type) are now a_top_level_array_of_scalars_is_masked, a_bare_top_level_scalar_is_masked and a_top_level_array_is_reached_by_sniffing_too.

Fixed in d2eeb6a.

Array elements inherit mask: self.mask, so an allowlisted key passes false down to every element — which contradicts the invariant in the comment just above. With success in the Adyen list:

{"success":["4111111111111111"]}  ->  {"success":["4111111111111111"]}

Objects do behave as documented. Passing mask: true to elements restores the invariant; arrays under a masked key are unaffected.

Confirmed, and taking it as you describe. The invariant is the right one to hold to: elements carry no key, so an operator can never name one — allowing success should reveal the scalar it names, not everything nested beneath it. Elements now always get mask: true.

Worth noting the two array rules pull in opposite directions and both are now pinned:

allowlist = [success]
{"success": true}                -> {"success": true}
{"success": ["4111111111111111"]} -> {"success": ["***"]}   <- this comment
{"tags": ["4111111111111111"]}    -> {"tags": ["***"]}      <- the original motivation

The first inheriting case was the reason mask was threaded into elements at all; it stays covered, since true is now passed unconditionally rather than inherited.

Also rewrote the Masked doc comment — it described mask as "carries the parent key's decision so array elements inherit it", which documented the bug rather than the intent. It now says mask is the decision for a value with no key of its own, cleared only by an allowlisted object entry.

Fixed in d2eeb6a.

This catch-all forwards Comment, DocType and PI verbatim, so a gateway that echoes the request inside a comment leaks it:

<r><!-- pan 4111111111111111 --><a>b</a></r>  ->  <r><!-- pan 4111111111111111 --><a>***</a></r>

Decl is structural and fine to copy through; these three aren't. Explicit arms that drop them, or emit *** in their place?

Confirmed — reproduced verbatim, including inside <!DOCTYPE>'s internal subset and a PI:

<r><!-- pan 4111111111111111 --><a>b</a></r>
<!DOCTYPE r [<!ENTITY pan "4111111111111111">]><r><a>b</a></r>
<r><?echo 4111111111111111?><a>b</a></r>

Went with dropping rather than emitting ***, except for comments where the placeholder is cheap and keeps the signal that a comment was there. Decl passes through as you say.

The more useful part of the change is the default. It was other => write_event(other) — forward anything unrecognised — which is why three variants leaked without anyone choosing that. It's now _ => {}, so an event this match doesn't know is dropped. On the next quick-xml bump (pinned at 0.31.0) a new variant fails closed instead of silently joining the passthrough.

Fixed in d2eeb6a.

b4431bc removed the unit tests and there's no #[cfg(test)] block, so Run Tests is green on 436 lines nothing exercises. Each leak I've flagged here reproduces in a few lines — minimum before merge:

• keyless top-level JSON: array of scalars, bare scalar
• arrays under an allowed key and under a denied key
• text/plain, CSV-shaped and binary bodies
• XML comments and DOCTYPE
• BOM-prefixed and newline-separated form bodies, both special-cased in mask_form and neither guarded
• an allowed key whose value is an object, to pin the invariant that holds

Taken in full. b4431bc is restored and ported — the config map is keyed by Box<str> now rather than ConnectorEnum, max_bytes is gone, and mask_connector_response takes &str — plus the cases you listed. 40 tests, and the toml dev-dependency comes back with them.

I wrote them before the fixes, against the code as it stood. 11 failed, one per bypass — which is the part of your comment that actually mattered, since a test written after a fix can't distinguish "passes because the fix works" from "passes because it never tested the leak". The failure output printed the PAN in the clear in every case:

a_bare_top_level_scalar_is_masked                          left: "\"4111111111111111\""
a_top_level_array_of_scalars_is_masked                     left: "[\"4111111111111111\",\"x\"]"
a_top_level_array_is_reached_by_sniffing_too
scalars_in_an_array_under_an_allowed_key_are_masked_too    left: "{\"success\":[\"4111111111111111\"]}"
a_plain_text_body_is_not_re_emitted_as_a_form_key          Transaction+declined+for+card+4111111111111111=***
a_csv_shaped_body_is_not_re_emitted_as_form_keys           1%2C4111111111111111%2CSUPERSECRET=***
a_declared_form_body_that_is_not_form_shaped_yields_the_stub   4111111111111111=***
a_binary_body_is_not_re_emitted_as_a_form_key
xml_comments_do_not_carry_their_content_through            <r><!-- pan 4111111111111111 --><a>b</a></r>
xml_doctype_does_not_carry_its_content_through             <!DOCTYPE r [<!ENTITY pan "4111111111111111">]>...
xml_processing_instructions_do_not_carry_their_content_through   <r><?echo 4111111111111111?><a>b</a></r>

The other 29 passed unchanged, which is the useful control — the ported coverage was not silently rewritten to fit the new behaviour.

Everything on your list is covered. Two notes on how:

• Assertions are on absence of the PAN, not only on output shape. assert_eq! pins how a value is spelt; assert!(!out.contains(PAN)) pins the property. Both, where the shape is worth fixing.
• Beyond your list: an allowlisted key holding a scalar (so the array rule can't over-mask), and config load accepting a name from each of the five enums.

BOM-prefixed and newline-separated form bodies both hold — a_bom_prefixed_body_is_still_parsed and a_newline_separated_form_body_is_masked_pair_wise.

Added in d2eeb6a.

is_known_connector accepts AuthenticatorConnectorEnum, but the lookup key comes from ConnectorVariant::get_connector_name() (connector_types.rs:352) and ConnectorVariant has no authenticator arm — so an authenticator entry passes startup validation and then never matches. The config comments still tell operators plaid is valid. Drop it from this check and the comments, or add the arm to get_connector_name().

Pushing back on this one — ConnectorVariant does have an authenticator arm, so the lookup resolves.

// connector_types.rs:354
pub enum ConnectorVariant {
    Payment(ConnectorEnum),
    Surcharge(SurchargeConnectorEnum),
    Payout(PayoutConnectorEnum),
    Frm(FrmConnectorEnum),
    Authenticator(AuthenticatorConnectorEnum),   // :359
}

// connector_types.rs:403
pub fn get_connector_name(&self) -> String {
    match self {
        ...
        ConnectorVariant::Authenticator(conn) => conn.to_string(),   // :409
    }
}

It arrived with #1975 (423af1e0d, "add Plaid as authenticator connector"), which is an ancestor of origin/main and entered this branch in 0cd38c022 on Aug 4 at 16:52 IST — before your review, so it isn't a rebase artefact. Likely a stale local checkout; the line you cited (:352) doesn't match either the pre- or post-#1975 numbering.

The flow also terminates somewhere real: merchant_authentication_flow_data.rs:74 implements set_masked_connector_response, so an authenticator response does reach masking rather than resolving to a key nothing ever queries.

So is_known_connector keeps AuthenticatorConnectorEnum and the config comments keep plaid. Happy to be shown otherwise if you're seeing something I'm not — but on this HEAD, dropping it would reject a connector that works.

The adjacent claim underneath it is worth pinning, so config_accepts_a_name_from_any_connector_enum now loads one name from each of the five enums (adyen, interpayments, deutschebank, kount, plaid) and asserts each resolves — that's what would actually catch this class of drift, in either direction.

#[strum(serialize_all = "snake_case")] on ConnectorEnum (connector_types.rs:60) applies to the derived Display as well as EnumString — which is why get_connector_name() gives adyen and the seeded config key resolves at all. Validating via FromStr is still right; the reasoning here just has it backwards.

Correct — the conclusion was right and the reasoning was wrong, which is the worse kind of comment to leave behind. #[strum(serialize_all = "snake_case")] (connector_types.rs:60) governs the derived Display alongside EnumString, which as you say is precisely why get_connector_name() yields adyen and the seeded key resolves.

What strum doesn't reach is serde, and that's the actual reason for the manual route: the connector enums carry no #[serde(rename_all)], so their serde derive would demand Adyen, while TOML keys and the config crate's env-var keys both arrive lowercased. Comment rewritten to say that.

Fixed in d2eeb6a.

enabled = true here and in sandbox.toml, gated independently of return_raw_connector_data, means the masked view reaches callers from the first deploy — which makes the bypasses I've flagged on connector_response_masking.rs live rather than latent. Can we default it false until those are closed and covered? The struct Default is already false, so it's config-only.

The comment block above also promises PAN/CVV/expiry stay masked regardless — true for keyed fields, not for the unkeyed paths.

Taking it. enabled = false in both sandbox.toml and production.toml, development.toml stays true. Config-only, as you note — the struct Default was already false.

I'd have argued the containment case is weaker now that the bypasses are closed in this same PR, but that's the wrong way round: the reason to ship it off is that turning it on returns connector response bytes to the caller, so a deployment should opt in once it has chosen its key lists — independent of whether any particular bypass is open. Added that as the comment above the flag.

On the comment block — you're right that it was a claim about keyed fields dressed up as an unconditional one. Rewritten to say what naming a key actually grants, and to name the unkeyed case explicitly rather than leave it implied:

# Naming a key here reveals only that key's own value: an object below it is re-decided key by
# key, and an array below it stays masked, since its elements have no key you could name.
# A key whose name looks like a full PAN, CVV, expiry or credential stays masked even if listed.
# Truncated values (cardSummary, last4, cardBin) are not covered — name them if needed.
# A body that is not JSON, XML or form-encoded has no keys to gate on, so it is replaced
# wholesale by a stub carrying only its size.

Applied to all three config files. Fixed in 52de180.